Skip to content
This repository has been archived by the owner on Nov 15, 2023. It is now read-only.

Implement AliasOrigin processing in XCVM #7245

Merged
merged 32 commits into from
Jun 5, 2023

Conversation

KiChjang
Copy link
Contributor

@KiChjang KiChjang commented May 17, 2023

Fixes #6668.

Processes the AliasOrigin instruction to allow for a given origin in the list of Aliasers to be substituted by a given target MultiLocation.

  • Implement functionality
  • Create XCM builder types as described in the issue
  • Tests
  • Benchmarks

cumulus companion: paritytech/cumulus#2680

@KiChjang KiChjang added A0-please_review Pull request needs code review. C1-low PR touches the given topic and has a low impact on builders. D3-trivial 🧸 PR contains trivial changes in a runtime directory that do not require an audit. T6-XCM This PR/Issue is related to XCM. B1-note_worthy Changes should be noted in the release notes labels May 17, 2023
runtime/kusama/src/lib.rs Outdated Show resolved Hide resolved
runtime/rococo/src/lib.rs Outdated Show resolved Hide resolved
xcm/xcm-builder/src/origin_aliases.rs Show resolved Hide resolved
xcm/xcm-builder/src/origin_aliases.rs Show resolved Hide resolved
@KiChjang
Copy link
Contributor Author

This will still require rerunning the benchmarks and generating the weight files.

@vstam1
Copy link
Contributor

vstam1 commented Jun 2, 2023

bot clean

@command-bot command-bot bot deleted a comment from paritytech-processbot bot Jun 2, 2023
@vstam1
Copy link
Contributor

vstam1 commented Jun 2, 2023

bot bench $ xcm kusama pallet_xcm_benchmarks::generic
bot bench $ xcm westend pallet_xcm_benchmarks::generic
bot bench $ xcm rococo pallet_xcm_benchmarks::generic

@vstam1
Copy link
Contributor

vstam1 commented Jun 2, 2023

bot clean

@kianenigma kianenigma self-requested a review June 2, 2023 16:00
xcm/xcm-builder/src/origin_aliases.rs Show resolved Hide resolved
@vstam1 vstam1 added T1-runtime This PR/Issue is related to the topic “runtime”. and removed T6-XCM This PR/Issue is related to XCM. labels Jun 5, 2023
@vstam1
Copy link
Contributor

vstam1 commented Jun 5, 2023

bot merge

@vstam1
Copy link
Contributor

vstam1 commented Jun 5, 2023

bot clean

@command-bot command-bot bot deleted a comment from paritytech-processbot bot Jun 5, 2023
@KiChjang
Copy link
Contributor Author

KiChjang commented Jun 5, 2023

bot merge

@paritytech-processbot
Copy link

Error: "Check reviews" status is not passing for paritytech/cumulus#2680

@vstam1
Copy link
Contributor

vstam1 commented Jun 5, 2023

bot merge

@paritytech-processbot paritytech-processbot bot merged commit bfb9e87 into master Jun 5, 2023
@paritytech-processbot paritytech-processbot bot deleted the kckyeung/xcm-alias-origin branch June 5, 2023 14:39
@@ -914,7 +914,15 @@ impl<Config: config::Config> XcmExecutor<Config> {
self.context.topic = None;
Ok(())
},
AliasOrigin(_) => Err(XcmError::NoPermission),
AliasOrigin(target) => {
let origin = self.origin_ref().ok_or(XcmError::BadOrigin)?;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@KiChjang @vstam1

I dont see any usage of AliasOrigin now, and cannot find any restrictions or recommendation for this instruction,
but if we have ClearOrigin before AliasOrigin in XCM message, then it wont work, I dont know maybe it is intended,

just thinking, what if we change:

let origin = self.origin_ref().ok_or(XcmError::BadOrigin)?;
if Config::Aliasers::contains(origin, &target) {

to:

let effective_origin = self.context.origin.as_ref().unwrap_or(&self.original_origin);
if Config::Aliasers::contains(effective_origin, &target) {

would that be a problem or issue?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I was thinking about that as well, but instead of using the original origin (could very easily lead to an origin escalation attack), we could also just have Aliasers::contains accept an Option<MultiLocation> as the origin ML. This will then allow custom logic to be used when the origin register is set to None.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The question with this approach now is to really ask ourselves whether AliasOrigin is a privileged operation or not. To be safe, I've erred on the side of caution and made it so, but it does not necessarily have to be the case if we document it properly, letting other chains know that they absolutely need to know how to handle the None case appropriately in their Aliasers implementation.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
A0-please_review Pull request needs code review. B1-note_worthy Changes should be noted in the release notes C1-low PR touches the given topic and has a low impact on builders. D3-trivial 🧸 PR contains trivial changes in a runtime directory that do not require an audit. T1-runtime This PR/Issue is related to the topic “runtime”.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

XCM: Origin Aliasing
7 participants